We want to add automation to eslint checks for the tree so we can be sure that as we get files up to spec we don't regress by accident. Some stuff is happening in mozreview but that only affects people who use mozreview. The basic things the job needs to do: pull the repo mach eslint --setup (installs eslint and plugins globally through npm) mach eslint Exit code should be appropriate for pass/fail. This would only need to run on a single platform, OSX is apparently the one that works best right now. I'm willing to do some legwork to get this up and running if that is helpful.

For linux use I think this will work (untested): * Install node (4.2.3) and npm * Pull the repo * cd <repodir> * npm install -g eslint * npm install -g eslint-plugin-react * npm link testing/eslint-plugin-mozilla * eslint [.js,.jsm,.jsx,.xml] .

(In reply to Dave Townsend [:mossop] from comment #1) > For linux use I think this will work (untested): > > * Install node (4.2.3) and npm > * Pull the repo > * cd <repodir> > * npm install -g eslint > * npm install -g eslint-plugin-react > * npm link testing/eslint-plugin-mozilla > * eslint [.js,.jsm,.jsx,.xml] . One thing we might want to do is put the latter parts of that in a script in the hg repo itself so we can modify it as we make changes to things like npm dependencies and supported filetypes.

Got this working on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5097cc0ed07 Probably a good starting point. I'm not sure if you want to make this Tier-1 or Tier-2 to start with. ATM it's failing.

ahal, you've been looking at linting as well; thought you'd be interested in this

Thanks, yes I'm interested. I think adding an eslint job is fine for now. Though just be aware, that my intention will be to replace it with a generic lint job in the future that runs all linters, not just eslint. I should be able to build off this eslint job, so I don't think there will be much duplicated effort. Two comments (both of which could be follow-ups): 1) it would be cool if the build jobs depended on this lint job, so builds don't even run if the linter fails (this may be hard with buildbot builds). We may also want to have more than one lint job, one for things that would cause compilation/syntax errors that would prevent builds, and another for style/soft errors that wouldn't block builds, but simply turn red. Probably a good idea to punt on this for now. 2) I don't think this should be optional on try, it should run with every push. It's very cheap to run and will prevent backouts from people who forgot to specify it. Though for try, we'd want to explicitly avoid my proposal in the first comment.

Review commit: https://reviewboard.mozilla.org/r/28225/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/28225/

Review commit: https://reviewboard.mozilla.org/r/28227/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/28227/

I'd like to start with this enabled on try at least and aggresively add any failing files to .eslintignore to get it green then enable it by default on the main trees.

https://reviewboard.mozilla.org/r/28227/#review25353 ::: testing/taskcluster/tasks/branches/base_job_flags.yml:108 (Diff revision 1) > + - gecko_eslint I think this should be `eslint_gecko`. ::: testing/taskcluster/tasks/tests/gecko_eslint.yml:16 (Diff revision 1) > + curl -s -S https://nodejs.org/download/release/v4.2.3/node-v4.2.3-linux-x64.tar.gz | tar --strip-components 1 -xz -C /usr/local && This is insecure. Need to verify checksums of files downloaded from external sources or you are opening us up to a MiTM attack. Alternatively, upload this archive to tooltool (preferred). ::: testing/taskcluster/tasks/tests/gecko_eslint.yml:21 (Diff revision 1) > + ./mach eslint --setup && I'm not a huge fan of running `mach eslint` in automation because it downloads from 3rd party servers. So if e.g. npm is down, the jobs fail. I /thought/ we had an automation policy where we tried not to rely on 3rd party services because it increases our surface area for failure and tree closures. ::: testing/taskcluster/tasks/tests/gecko_eslint.yml:22 (Diff revision 1) > + eslint --ext [.js,.jsm,.jsx,.xml] -f json . | python testing/eslintparser.py eslint supports different output formats. I don't suppose you could write an eslint plugin that emitted native output so we didn't have to post-process via Python? Did you investigate this?

Comment on attachment 8699273 [details] MozReview Request: Bug 1229588: Update taskcluster desktop-test image to include node and eslint modules. f?dustin https://reviewboard.mozilla.org/r/28225/#review25355 I'm not a huge fan of testing/ as the location for this file, as that's generally not a place where you find .py files. (The things there are ancient.) But I'm at a loss to suggest something better. Someone in #ateam could probably suggest a better location. ::: testing/eslintparser.py:16 (Diff revision 1) > +for file in files: `file` is a built-in global symbol in Python. Please use `f` or something else. ::: testing/eslintparser.py:27 (Diff revision 1) > + print("TEST-UNEXPECTED-FAIL | %s | Unparseable eslint error" % path) Something we've learned from experience is to check the case where there is no output and error. i.e. if there are no results, abort. Yes, we've had periods in automation where there was no output because nothing was being executed and we interpretted this as success!

(In reply to Gregory Szorc [:gps] from comment #10) > I'm not a huge fan of testing/ as the location for this file, as that's > generally not a place where you find .py files. (The things there are > ancient.) But I'm at a loss to suggest something better. > > Someone in #ateam could probably suggest a better location. I was thinking something like 'tools/lint'.

> (Diff revision 1) > > + ./mach eslint --setup && > > I'm not a huge fan of running `mach eslint` in automation because it > downloads from 3rd party servers. So if e.g. npm is down, the jobs fail. I > /thought/ we had an automation policy where we tried not to rely on 3rd > party services because it increases our surface area for failure and tree > closures. Would this be a reasonable thing to bake into the regular desktop build image? Or should we create a new image for running the linters?

(In reply to Chris AtLee [:catlee] from comment #12) > > (Diff revision 1) > > > + ./mach eslint --setup && > > > > I'm not a huge fan of running `mach eslint` in automation because it > > downloads from 3rd party servers. So if e.g. npm is down, the jobs fail. I > > /thought/ we had an automation policy where we tried not to rely on 3rd > > party services because it increases our surface area for failure and tree > > closures. > > Would this be a reasonable thing to bake into the regular desktop build > image? Or should we create a new image for running the linters? There's really two things that this script is downloading: * An updated node, the current image includes the old 0.12 version while 4.2.3 is the new stable version. * A bunch of npm packages for eslint and various dependencies for our custom plugin. For the first it might make sense to include in the default image or as a new image, we're unlikely to want to update node often. For the npm packages I'm less sure, right now we're actively improving our mozilla eslint plugin adding new features which often need new npm packages to work. I can think of three changes to the dependency set in the past few weeks. I don't know the relative cost of changing the image vs. uploading the new set of packages to tooltool, the latter sounds likely easier though if I can figure out how to get tooltool downloading working in there.

Updating the images is easy with taskcluster. Especially if we make a new 'lint' image (as modifying it wouldn't impact other jobs). Keep in mind that each linter will have its own set of dependencies, e.g pyflakes requires a bunch of python libraries. Baking these dependencies into a "lint" image (or just adding them to the existing one) seems easier than uploading tens of packages to tooltool.

If there's an apt package, and we wanted all dependencies installed for globally for all test jobs, it's as easy as adding a line here: https://dxr.mozilla.org/mozilla-central/source/testing/docker/ubuntu1204-test/system-setup.sh#12 (and re-building/uploading the new images) Otherwise, we'd need to make a new image that depends on 'desktop-test' or similar and install the packages there.

(In reply to Andrew Halberstadt [:ahal] from comment #15) > If there's an apt package, and we wanted all dependencies installed for > globally for all test jobs, it's as easy as adding a line here: > https://dxr.mozilla.org/mozilla-central/source/testing/docker/ubuntu1204- > test/system-setup.sh#12 The node we need isn't in apt for ubuntu 12.04, nor are the node packages we rely on so we'll have to do the extra work. Is this something I can do with guidance?

Creating a new image isn't too hard, and only requires changes to m-c. You can look at testing/docker/desktop-test as an example (and docker's Dockerfile documentation). You might need help getting your new image uploaded, or getting your docker account added to the mozilla org. The question I don't know how to answer is, should this job have it's own image, or should it re-use the existing base image. If the latter is ok, then you can install whatever you need in system-setup.sh linked from comment 15. Whether that's via apt repositories, or wget + tar. Greg/Dustin, do you guys have any strong preferences on whether to make a new image for a lint job, or just re-use an existing one? The implication is that it will need to contain whatever dependencies the linters have.

I would personally like to see one unified tester image for desktop instead of one off images. If adding the packages to desktop-test doesn't cause any other issues or a lot of bloat, I vote for it to be in that image.

seconded

(In reply to Andrew Halberstadt [:ahal] from comment #15) > If there's an apt package, and we wanted all dependencies installed for > globally for all test jobs, it's as easy as adding a line here: > https://dxr.mozilla.org/mozilla-central/source/testing/docker/ubuntu1204- > test/system-setup.sh#12 It looks like that is already downloading node and installing it, but its using an older version than we need. Can I safely change that to point to a newer version? https://dxr.mozilla.org/mozilla-central/source/testing/docker/ubuntu1204-test/system-setup.sh#163

I believe it should be safe. The nice thing about taskcluster is you can push your changes to try to test it out. Greg Arndt recently added a mechanism that will automatically build and upload an image to dockerhub if you change it, though I haven't used it yet myself and I believe it still has some limitations if there is Dockerfile inheritance involved. You could ping him here or in #taskcluster for more info though.

The new feature will not upload to docker hub, rather it will upload it as a task artifact that other tasks can then reference. Some information is here [1] and here [2] about it. Unfortunately if the changes are being made to ubuntu1204-test, then this new feature will not help since it does not handle inheritance currently (desktop-test is the image used in production which inherits ubuntu1204-test I believe). [1] http://blog.gregarndt.com/taskcluster/2015/12/09/images-on-push/ [2] https://hg.mozilla.org/mozilla-central/file/tip/testing/docker/README.md#l19

Ah, in that case it's still possible to test out on try, just a bit more work. Luckily :jmaher documented his experience doing exactly what you need to do in a blog post: https://elvis314.wordpress.com/2015/11/11/adventures-in-task-cluster-running-a-custom-docker-image/ It's not super nice.. but it still beats updating an image in buildbot.

Ok it turns out that with the updated node package the install of the npm module taskcluster-npm-cache in desktop-test fails, likely because it is either failing to compile against the new node headers or because the new node requires a newer compiler than Ubuntu 12 provides (https://github.com/nodesource/distributions/blob/master/OLDER_DISTROS.md) :(

https://reviewboard.mozilla.org/r/28227/#review25353 > eslint supports different output formats. I don't suppose you could write an eslint plugin that emitted native output so we didn't have to post-process via Python? Did you investigate this? Yeah I've just figured out how to do that so I'll switch it.

https://reviewboard.mozilla.org/r/28225/#review25355 > Something we've learned from experience is to check the case where there is no output and error. i.e. if there are no results, abort. > > Yes, we've had periods in automation where there was no output because nothing was being executed and we interpretted this as success! The problem here is that eslint outputs nothing in case of success, so we can't really do that.

Comment on attachment 8699273 [details] MozReview Request: Bug 1229588: Update taskcluster desktop-test image to include node and eslint modules. f?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28225/diff/1-2/

Comment on attachment 8699274 [details] MozReview Request: Bug 1229588: Add taskcluster test for eslint. r?ahal Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28227/diff/1-2/

(In reply to Dave Townsend [:mossop] from comment #27) > Comment on attachment 8699273 [details] > MozReview Request: Bug 1229588: Update taskcluster desktop-test image to > include node and eslint modules. f?dustin > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/28225/diff/1-2/ *reviewboard sigh* This is just for feedback, there are more notes in the commit description if you can find it in reviewboard :(

https://reviewboard.mozilla.org/r/28227/#review26873 ::: testing/taskcluster/tasks/branches/base_job_flags.yml:108 (Diff revision 2) > + - eslint_gecko nit: we're tending strongly toward dashes (`-`) here, rather than underscore (`_`) ::: testing/taskcluster/tasks/tests/eslint_gecko.yml:3 (Diff revision 2) > + from: 'tasks/build.yml' Does it make sense to have this based on `build.yml`? It's a weird case between build and test, isn't it -- doesn't require a binary to test, yet doesn't produce one, either. I wonder if we should have another name for this? Specifically, the public/build artifact definition in `build.yml` is inappropriate for this task, I think. ::: testing/taskcluster/tasks/tests/eslint_gecko.yml:11 (Diff revision 2) > + image: '{{#docker_image}}desktop-test{{/docker_image}}' Is this the image you want? The checkout-sources.sh script is in the `desktop-build` image, instead. If you can't find an appropriate image for this task, it might make sense to build a new one, perhaps based on [node](https://hub.docker.com/_/node/) and with eslint installed in the image. ::: testing/taskcluster/tasks/tests/eslint_gecko.yml:26 (Diff revision 2) > + platform: linux64 "Platform" is one of those words that means 1,000 things at Mozilla, but in `base_jobs.yml` you set platform to `ESLint`, yet here it's `linux64`. Should those be the same?

(In reply to Dustin J. Mitchell [:dustin] from comment #30) > If you can't find an appropriate image for this task, it might make sense to > build a new one, perhaps based on [node](https://hub.docker.com/_/node/) and > with eslint installed in the image. That would probably be straightforward, though I still need the checkout_sources script to be able to get the source to check. ahal, I know you were thinking of making this be a generic image for linting work, does that work for you or do we still need something more generic? > ::: testing/taskcluster/tasks/tests/eslint_gecko.yml:26 > (Diff revision 2) > > + platform: linux64 > > "Platform" is one of those words that means 1,000 things at Mozilla, but in > `base_jobs.yml` you set platform to `ESLint`, yet here it's `linux64`. > Should those be the same? I don't really know what those fields do, I just copied that from catlee's original patch. What should I set it to?

Regarding platform, I'm not entirely sure -- I think the one in the task description affects what row the job appears in on Treeherder.

I'd like to eventually have this job running all linters, so we don't have N nearly identical lint jobs in treeherder. So something generic would be nice.. That being said, I can change this up when the time comes, so if using a Node specific image is easier for now, I'd say just go for it.

Adds a new lint docker image for linting tools and adds an eslint-gecko task that uses it to run eslint over the tree. Review commit: https://reviewboard.mozilla.org/r/30229/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30229/

(In reply to Dave Townsend [:mossop] from comment #34) > Created attachment 8705884 [details] > MozReview Request: Bug 1229588: Add a taskcluster test for eslint. r?ahal > > Adds a new lint docker image for linting tools and adds an eslint-gecko task > that uses it to run eslint over the tree. > > Review commit: https://reviewboard.mozilla.org/r/30229/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/30229/ I think we're there now. I ended up copying build.yml to lint.yml for a base for linting tasks and removed everything that didn't look useful and didn't break the build. I don't understand a lot of the yml so please correct me where I've made mistakes. Also created an in-tree lint image so no need to publish the image anywhere! This passes (well runs and shows the expected failures) on try now, on tree herder it shows a new platform, "lint". https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b5f6689850a&selectedJob=15239517

Comment on attachment 8705884 [details] MozReview Request: Bug 1229588: Add a taskcluster test for eslint. r?dustin https://reviewboard.mozilla.org/r/30229/#review27011 I like this approach. I'm not really sure if I have review powers in here or not though. I'd feel better if Dustin took another look anyway. ::: testing/taskcluster/tasks/lint.yml:16 (Diff revision 1) > + workerType: b2gbuild I believe this determines what type of AWS instance the job will run on, and we probably don't need a machine as beefy as b2gbuild's do. Normally you can see the worker-types here, but there seems to be an expired cert: https://tools.taskcluster.net/aws-provisioner/ Dustin, any idea what a good worker type for this might be? ::: testing/taskcluster/tasks/lint.yml:30 (Diff revision 1) > + maxRunTime: 7200 This is probably way longer than needed for linting. Can we do half an hour tops? ::: testing/taskcluster/tasks/tests/eslint-gecko.yml:25 (Diff revision 1) > + locations: > + build: null > + tests: null Can this section be removed? Or does that cause an error.

Comment on attachment 8705884 [details] MozReview Request: Bug 1229588: Add a taskcluster test for eslint. r?dustin Dustin, I'm not 100% sure if I should be reviewing stuff like this or not. I'd at least appreciate a second pair of eyes on the new image.

Comment on attachment 8705884 [details] MozReview Request: Bug 1229588: Add a taskcluster test for eslint. r?dustin https://reviewboard.mozilla.org/r/30229/#review27047 Andrew's concerns were good -- I just added some extra detail. ::: testing/taskcluster/tasks/lint.yml:16 (Diff revision 1) > + workerType: b2gbuild Let's use `b2gtest` for now. Once this scales up, we can consider a dedicated workerType. The factors to balance are: * a new workerType means nothing else uses those instances, so depending on load we may end up with instances running idle * sharing a workerType but using different Docker images for the tasks can lead to contention for disk space for the images ::: testing/taskcluster/tasks/tests/eslint-gecko.yml:25 (Diff revision 1) > + locations: > + build: null > + tests: null If this does cause errors, you may need to patch the mach command to recognize linting tasks as some kind of source-code-test job, which neither requires a build artifact, nor produces one.

(In reply to Dustin J. Mitchell [:dustin] from comment #38) > ::: testing/taskcluster/tasks/tests/eslint-gecko.yml:25 > (Diff revision 1) > > + locations: > > + build: null > > + tests: null > > If this does cause errors, you may need to patch the mach command to > recognize linting tasks as some kind of source-code-test job, which neither > requires a build artifact, nor produces one. It does cause an error but it's not entirely clear to me what I should be changing. Here is where it validates that every task has locations.built and locations.tests declared: https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/taskcluster_graph/build_task.py#27 Can you elaborate more on what you're looking for?

So we have "build tasks" and "test tasks" -- the former are expected to take some VCS revisions and produce some artifacts (firefox binary, tests.zip, etc.). The latter are expected to run after a build task, and take as input links to the artifacts produced by the build tasks. A lint task doesn't fit either of these molds. That's what's making this a bit weird. I can see something like 'make check' falling into the same category (although that does require a configure and compile first..). We might also run pylint or some other source-code sanity checks. I do see support for post_build tasks -- maybe linting makes sense there? https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/mach_commands.py#469 Otherwise, maybe we should add a new kind of task to the graph, perhaps a "check task"? That would be handled outside of the loop over build tasks (https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/mach_commands.py#388). Maybe this is scope creep -- if you can get things working with the null's, then file another bug to implement check tasks, I think that would be OK. The bug fairies will take care of it!

(In reply to Dustin J. Mitchell [:dustin] from comment #40) > Maybe this is scope creep -- if you can get things working with the null's, > then file another bug to implement check tasks, I think that would be OK. > The bug fairies will take care of it! I think I'd like to do that, at this point other than that issue the task is running to completion and showing us failures and until we turn it on the number of failures will just keeps rising. Plus it's probably better for someone who knows this better than me to figure out what is best there. So I'll put up the last patch and separately get the existing failures fixed then presumably we need another bug to turn this on in the nightly trees?

Comment on attachment 8705884 [details] MozReview Request: Bug 1229588: Add a taskcluster test for eslint. r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30229/diff/1-2/

Comment on attachment 8705884 [details] MozReview Request: Bug 1229588: Add a taskcluster test for eslint. r?dustin https://reviewboard.mozilla.org/r/30229/#review27361

Comment on attachment 8705884 [details] MozReview Request: Bug 1229588: Add a taskcluster test for eslint. r?dustin https://reviewboard.mozilla.org/r/30229/#review27403 ::: testing/taskcluster/tasks/tests/eslint-gecko.yml:23 (Diff revision 2) > + eslint --plugin html --ext [.js,.jsm,.jsx,.xml,.html] -f tools/lint/eslint-formatter . :nalexander just embedded some shell script into a task description, and I told him it should be in mozharness. He called me out for being inconsistent :) I think this is OK here, because (a) it's so simple and (b) it doesn't use mach or anything else that's already well-supported by mozharness. I don't want to invent a new kind of harness for running jobs, though, so I think if this gets any more complex, it should probably become a mozharness script -- Mozharness isn't perfect, but it's what we use for just about everything. There's value in consistency.

Comment on attachment 8705884 [details] MozReview Request: Bug 1229588: Add a taskcluster test for eslint. r?dustin apparently failing to *re*-check "Ship It!" causes the r+ to be erased, sorry..